Add compile-time field iteration for serialization - #1219
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9e2100348
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String name = call.getFuncName(); | ||
| if ("forFields".equals(name) || "mapFields".equals(name)) { | ||
| calls.add(call); |
There was a problem hiding this comment.
Avoid hijacking user-defined field helper calls
Any existing package that defines a regular unqualified forFields or mapFields function now has every call collected before name resolution. For example, function forFields(int x) ... followed by forFields(1) is reported as being outside an instance context instead of resolving to the user function, introducing a source-breaking global reservation of two ordinary identifiers. Restrict the rewrite to explicit intrinsic syntax or markers so resolvable user calls remain valid.
Useful? React with 👍 / 👎.
| public void visit(ExprVarAccess access) { | ||
| super.visit(access); | ||
| if (access.getVarName().equals(nameParameter) || access.getVarName().equals(valueParameter)) { | ||
| accesses.add(access); |
There was a problem hiding this comment.
Preserve bindings when substituting closure parameters
When the callback body contains a nested closure whose parameter reuses name or value, this visitor descends into that closure and replaces its bound ExprVarAccess solely by spelling. For example, forFields((name, value) -> consume((int value) -> value)) silently changes the inner closure to return this.<field> rather than its argument for every field, miscompiling valid code; make substitution scope-aware and do not replace references shadowed by nested declarations.
Useful? React with 👍 / 👎.
| @Test | ||
| public void serializesAndDeserializesFieldsWithoutRuntimeReflection() throws IOException { | ||
| test() | ||
| .testLua(true) |
There was a problem hiding this comment.
Exercise the feature test on Jass too
This test calls .testLua(true), whose setter also sets luaOnly = true in WurstScriptTest, so the harness skips every Jass translation/interpreter variant. The new compiler feature therefore has no committed regression coverage on Jass; enable Lua without luaOnly or add a separate Jass execution test.
AGENTS.md reference: AGENTS.md:L215-L221
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d78b456b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int statementIndex = statements.indexOf(call); | ||
| statements.remove(statementIndex); |
There was a problem hiding this comment.
Preserve callback parameter diagnostics during expansion
When either placeholder has an explicit type, removing the call also removes its WShortParameter nodes before validation, so invalid annotations are silently accepted. For example, __wurst_mapFields((NoSuch name, NoSuch value) -> value) compiles and runs despite both referenced types being undefined. Validate these declarations or reject explicit annotations before discarding the closure so ordinary type diagnostics are not bypassed.
AGENTS.md reference: AGENTS.md:L84-L85
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6304748a09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| int statementIndex = statements.indexOf(call); | ||
| statements.remove(statementIndex); |
There was a problem hiding this comment.
Preserve callback validation for fieldless classes
When a class has no non-static fields, including when all its fields are static, this removes the intrinsic but the loop emits no replacement AST. The callback is therefore never name- or type-checked; for example, __wurst_forFields((name, value) -> noSuchFunction(name, value)) compiles and executes in an empty class even though the function is undefined. Preserve a validation path or diagnose fieldless use rather than silently deleting invalid code.
AGENTS.md reference: AGENTS.md:L82-L85
Useful? React with 👍 / 👎.
| String nameParameter = closure.getShortParameters().get(0).getName(); | ||
| String valueParameter = closure.getShortParameters().get(1).getName(); |
There was a problem hiding this comment.
Reject duplicate field-placeholder names
When both closure parameters use the same identifier, such as __wurst_mapFields((value, value) -> 42), deleting the closure before normal validation suppresses the standard duplicate-name diagnostic, and the program is accepted. If that identifier is referenced, substitution also always treats it as the first, field-name parameter, making the value placeholder inaccessible; reject duplicate placeholder names before expansion.
AGENTS.md reference: AGENTS.md:L82-L85
Useful? React with 👍 / 👎.
|
@codexy review |
|
@codex review |
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b92f5d0fa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!shadowed.contains(access.getVarName()) | ||
| && (access.getVarName().equals(nameParameter) || access.getVarName().equals(valueParameter))) { | ||
| accesses.add(access); |
There was a problem hiding this comment.
Respect local bindings when substituting block callbacks
When a __wurst_mapFields callback uses a begin ... end expression, local variables and loop variables can shadow either placeholder, but this visitor excludes only parameters of nested closures. For example, (name, value) -> begin; let value = 42; return value; end has its return access replaced with this.<field>, so the map silently preserves the old field value instead of assigning 42. Resolve accesses against the original WShortParameter definitions or track all lexical declarations during substitution.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c84d81a148
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!shadowedScopes.isEmpty() | ||
| && (localVarDef.getName().equals(nameParameter) | ||
| || localVarDef.getName().equals(valueParameter))) { | ||
| shadowedScopes.peek().add(localVarDef.getName()); |
There was a problem hiding this comment.
Keep for-loop bindings inside their loop
When a block callback declares a for variable with either placeholder name, this newly added handler records the loop's LocalVarDef in the surrounding WStatements scope. Because the generated visitor processes that declaration before the loop's iterable/range expressions and leaves it recorded through subsequent statements, legitimate placeholder references in those expressions or after the loop are not substituted; once the outer closure is removed, they become unresolved or bind incorrectly. Give loop variables a dedicated scope covering only their body and add a regression for a placeholder reference after such a loop.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b90e988e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public void visit(LocalVarDef localVarDef) { | ||
| super.visit(localVarDef); | ||
| if (!shadowedScopes.isEmpty() | ||
| && (localVarDef.getName().equals(nameParameter) | ||
| || localVarDef.getName().equals(valueParameter))) { | ||
| shadowedScopes.peek().add(localVarDef.getName()); |
There was a problem hiding this comment.
Bind placeholder substitutions to actual declarations
The newly added scope tracker still mishandles an access that precedes a same-named local declaration because it records the local only after super.visit(localVarDef). WStatements exposes every LocalVarDef in the block's name scope, so in a callback such as let old = value; let value = 42; return old, the first access binds to the local and should receive the usual use-before-initialization diagnostic; this expansion instead replaces it with this.<field> and deletes the closure before validation. Resolve each access against its declaration, or account for all block-local declarations before visiting the block.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec5b0b868
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| List<GlobalVarDef> fields = new ArrayList<>(); | ||
| for (GlobalVarDef field : classDef.getVars()) { |
There was a problem hiding this comment.
Include inherited fields in field iteration
When the intrinsic is used in a subclass, classDef.getVars() contains only fields declared directly on that class, so inherited instance fields are silently omitted from both serialization and deserialization. A subclass with one local field and one inherited field therefore emits callbacks only for the local field, corrupting round trips without a diagnostic; collect the accessible instance fields across the superclass hierarchy in a deterministic order.
Useful? React with 👍 / 👎.
| ClassDef classDef = call.attrNearestClassDef(); | ||
| if (classDef == null || !call.attrIsDynamicContext()) { | ||
| call.addError(call.getFuncName() + " can only be used in an instance method or constructor."); |
There was a problem hiding this comment.
Expand intrinsics after module methods are instantiated
When __wurst_forFields or __wurst_mapFields is declared inside a module method, parsing always reports that it is outside an instance context because the method has no nearest ClassDef until ModuleExpander copies it into a consuming class. This prevents reusable serializer modules from using the feature at all; defer expansion until module instantiation supplies the concrete class context.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c924a3e4f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (owner instanceof ModuleDef) { | ||
| // Module bodies are templates. Their copies were made by ModuleExpander; validate and | ||
| // expand those concrete copies instead of type-checking this uninstantiated template. | ||
| statements.remove(call); |
There was a problem hiding this comment.
Preserve intrinsic calls in module templates
After an initial full LSP check, this permanently removes the intrinsic from the ModuleDef. If a later reconciliation reparses only a separate consuming compilation unit, ModelManagerImpl.clearModuleInstantiation clears that class's generated module copy and ModuleExpander recreates it from the now-stripped template, silently leaving the serializer method empty. Keep the template intact while avoiding direct expansion/validation, so subsequent module instantiations can still copy the intrinsic.
AGENTS.md reference: AGENTS.md:L129-L132
Useful? React with 👍 / 👎.
| if (superType != null) { | ||
| collectInheritedFields(superType, fields, visited); | ||
| } | ||
| addInstanceFields(type.getClassDef().getVars(), fields); |
There was a problem hiding this comment.
Include instantiated module fields in field iteration
collectInheritedFields only visits fields declared directly on class definitions, but ClassTranslator.translateVars recursively translates module-instantiation variables into the same runtime class fields. Consequently, a class using a module with instance state silently omits that state from __wurst_forFields and leaves it unchanged in __wurst_mapFields; a class whose fields all come from modules is rejected as fieldless. Traverse the class's module instantiations, including nested modules, when collecting fields.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 514b0d7ebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isUninstantiatedModuleFieldIteration(call) | ||
| && call.getParent() instanceof WStatements statements) { | ||
| int index = statements.indexOf(call); | ||
| statements.remove(index); |
There was a problem hiding this comment.
Defer removals until after visiting module templates
When a module method contains two adjacent field-iteration intrinsics, removing the first call here mutates the WStatements list while DefaultVisitor is iterating it. The next iterator access throws ConcurrentModificationException, aborting the compiler and leaving the template partially detached; collect the calls during traversal and remove them only after the visitor completes.
Useful? React with 👍 / 👎.
| collectInheritedFields(superType, fields, visitedClasses, visitedModules); | ||
| } | ||
| addModuleFields(type.getClassDef().getModuleInstanciations(), fields, visitedModules); | ||
| addInstanceFields(type.getClassDef().getVars(), fields); |
There was a problem hiding this comment.
Exclude inaccessible private fields from inherited iteration
When a superclass declares a private instance field, this recursively adds it to a subclass's expansion even though the generated this.<field> access occurs in the subclass context. Name resolution then reports that the field is private, so using either intrinsic in an otherwise valid subclass fails compilation; inherited collection should exclude fields that are inaccessible from the concrete class.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57b3edb32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| int statementIndex = statements.indexOf(call); | ||
| statements.remove(statementIndex); |
There was a problem hiding this comment.
Preserve intrinsic nodes across incremental checks
When an imported superclass or module changes, ModelManagerImpl rechecks dependent compilation units by clearing attributes/module instantiations without reparsing their source. A direct class intrinsic has already been permanently removed here and, unlike the restored module-template calls, is never saved, so the next expansion finds nothing and retains statements for the old field layout; subsequent LSP builds can silently omit newly added fields or reference removed ones. Preserve or reconstruct direct intrinsic calls during reconciliation.
AGENTS.md reference: AGENTS.md:L129-L132
Useful? React with 👍 / 👎.
| statements.remove(statementIndex); | ||
|
|
||
| for (GlobalVarDef field : fields) { | ||
| Expr fieldAccess = fieldAccess(call.getSource(), field.getName()); |
There was a problem hiding this comment.
Qualify accesses to fields from sibling modules
Classes may legally contain sibling module instantiations whose fields have the same spelling and distinguish them with qualified accesses such as B.x and C.x. Collecting both declarations but generating this.x for each makes both expanded callbacks read or update the same field; after assigning different values to the two module fields, field iteration silently produces the wrong result. Carry the declaring module path into the generated access (and field key) rather than retaining only field.getName().
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a0a735b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isUninstantiatedModuleFieldIteration(call) | ||
| && call.getParent() instanceof WStatements statements) { | ||
| int index = statements.indexOf(call); | ||
| detached.add(new DeferredModuleCall(statements, index, call)); |
There was a problem hiding this comment.
Validate field intrinsics in unused module templates
When a module is never instantiated, no concrete copy exists to validate, yet this removes its intrinsic from the AST before WurstValidator runs, while expansion explicitly skips calls owned by ModuleDef. Consequently, malformed calls such as __wurst_forFields(x -> x) in an unused module compile without the expected closure diagnostic; validate the intrinsic's context and callback shape in the template even if field-dependent expansion must remain deferred.
AGENTS.md reference: AGENTS.md:L82-L85
Useful? React with 👍 / 👎.
| boolean privateFromAnotherClass = field.attrIsPrivate() | ||
| && concreteClass != null | ||
| && field.attrNearestClassDef() != concreteClass; | ||
| if (!field.attrIsStatic() && !privateFromAnotherClass) { |
There was a problem hiding this comment.
Exclude inaccessible private module fields
When a used module declares a private instance field, the copied field's attrNearestClassDef() is the consuming class, so this condition incorrectly considers it accessible and generates an access such as M.hidden. Name resolution then reports that the variable cannot be resolved for the module receiver, preventing either field intrinsic from compiling in an otherwise valid class; determine visibility relative to the defining module instantiation or omit inaccessible module fields.
Useful? React with 👍 / 👎.
| private static final Map<CompilationUnit, List<DeferredModuleCall>> DETACHED_DIRECT_CALLS = | ||
| Collections.synchronizedMap(new WeakHashMap<>()); |
There was a problem hiding this comment.
Avoid retaining compilation units through weak-map values
When callers repeatedly compile transient models without invoking the language-server-specific clearDirectFieldIterations, every successful direct intrinsic leaves an entry here for the lifetime of the JVM. The WeakHashMap does not make these entries collectible because each value holds WStatements and calls whose parent chain strongly reaches the CompilationUnit used as the weak key, so whole ASTs accumulate across compilations; store restoration state without a path back to the key or provide cleanup in the general compiler lifecycle.
AGENTS.md reference: AGENTS.md:L91-L91
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70ddd1b031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boolean privateFromAnotherModule = field.attrIsPrivate() && declaringModule != null; | ||
| if (!field.attrIsStatic() && !privateFromAnotherClass && !privateFromAnotherModule) { |
There was a problem hiding this comment.
Retain private fields for intrinsics declared in their module
When the intrinsic itself is declared in a module method, that method is allowed to access the module's private state, but this blanket check excludes every private module field without considering the call's originating module. The new declaringModule != null condition is fresh evidence beyond the earlier inaccessible-field report: a module with private hidden and public visible fields now silently serializes only visible, while a module containing only hidden fails with “requires at least one instance field.” Track the intrinsic's module origin and exclude private fields only for foreign module contexts.
AGENTS.md reference: AGENTS.md:L82-L85
Useful? React with 👍 / 👎.
| if (assignsResult) { | ||
| expanded = Ast.StmtSet(call.getSource(), (LExpr) fieldAccess, implementation); |
There was a problem hiding this comment.
Exclude immutable fields from map expansion
In an ordinary instance method, __wurst_mapFields emits this assignment for every collected field, including instance constants and readonly fields that the current class cannot write. Thus a valid data class containing constant int schema = 1 and a mutable payload cannot use the intrinsic at all—even a callback returning the old value produces “Cannot assign a new value to constant schema.” Build a writable field list for mapFields or issue a targeted intrinsic diagnostic while retaining such fields for read-only forFields iteration.
Useful? React with 👍 / 👎.
What changed
Adds two compile-time-expanded field operations for instance methods and constructors:
__wurst_forFieldsemits one direct callback expression per instance field.__wurst_mapFieldsemits one direct assignment per field using the callback result. Static fields are excluded and declaration order is preserved. The__wurst_spelling makes the compiler intrinsic explicit and leaves ordinary user-definedforFields/mapFieldsfunctions untouched.Why
Wurst save/load libraries currently require handwritten serialization and deserialization code for every field. Runtime reflection would add metadata and lookup overhead on Warcraft III targets. Expanding field operations before type checking gives reflection-like ergonomics while preserving overload resolution and producing the same direct field accesses as handwritten code.
This is intentionally a narrow draft/prototype so the API and limitations can be discussed before broadening it (for example inheritance, module fields, opt-out annotations, or schema/version behavior).
Impact
Validation
./gradlew test, 8m40s).git diff --checkpasses.